-
Notifications
You must be signed in to change notification settings - Fork 24
1333: sync requirements.txt and pyproject.toml #1373
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
1333: sync requirements.txt and pyproject.toml #1373
Conversation
|
I have validated that on python3.12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be a good idea to separate our requirements into runtime dependancies in the requirement.txt and have a requirement-dev.txt with the development tools/linters/etc that also references '-r requirements.txt' in it to limit package size of our executable. That way we can pull in the runtime dependencies into the build while excluding all the other packages we dont need. We would also maybe need to update documents to reference requirement-dev.txt when installing dependencies in our readme. What do you think about this proposal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR pushes a change that synchronizes the pyproject.toml file with the requirements.txt file to keep both coherent. The validation was done by installing the CDISC rules engine project locally using pip. The install was successful and had same version as in requirements.txt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this only needs the requirements-dev.txt if we reference requirements.txt in the dev file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry not getting your point... we basically need to install dev dependencies here to make pytest work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is in reference to what i wrote below:
"I would add a line -r requirement.txt to this file at the top so users can install all requirements for dev and runtime with just pip install requirement-dev.txt"
If installing dev requirements has a reference to requirement.txt, then we only need pip install -r requirements-dev.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha, just updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm -- just get rid of this one readme line and we can merge
README.md
Outdated
|
|
||
| - Install the requirements. | ||
|
|
||
| `python -m pip install -r requirements.txt` # From the root directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eliminate this line-- just need to install python -m pip install -r requirements-dev.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, nice work
* Applying jsonata * Need the builder * reporting works * jsonata can now be direct value of the Check property * Ability to load custom functions * fix unit test missing arg * Fix unit test missing arg * jsonata unit test * Added docs and fixed report message * process rule once for jsonata * Update result property names * fixed json for utf8, added error handling * error handling for jsonata loading * Schema fix * Added builtin jsonata functions, cli multi function paths, better error handling and tests * removed unneeded jsonata files. fixed md indentation * jsonata ignore scope * tweak output variables mapping * jsonata scope readme update * Add _path preprocessing * format fix * fix merge bug * missing test new args
* Initial plan * Convert all RepresentationInterface classes to dataclasses and replace to_representation with as_dict Co-authored-by: gerrycampion <[email protected]> * Refactor dataclasses according to feedback: use union syntax, remove field init=False, rename underscore fields Co-authored-by: gerrycampion <[email protected]> * Fix type annotations and Union syntax according to feedback Co-authored-by: gerrycampion <[email protected]> * Refactor ValidationErrorContainer to remove __init__ and update all constructor calls to use direct field assignments Co-authored-by: gerrycampion <[email protected]> * Fix ExecutionStatus enum handling in ValidationErrorContainer and RuleValidationResult Co-authored-by: gerrycampion <[email protected]> * Add fallback handling for ExecutionStatus enum lookups to prevent None assignment Co-authored-by: gerrycampion <[email protected]> * Fix VariableMetadataContainer typing issues and add enhanced ExecutionStatus safety checks Co-authored-by: gerrycampion <[email protected]> * Fix type annotation inconsistencies in RuleValidationResult field declarations Co-authored-by: gerrycampion <[email protected]> * Fix critical dataclass field ordering issues causing GitHub Actions mypy failures Co-authored-by: gerrycampion <[email protected]> * Revert RepresentationInterface removal and change as_dict() back to to_representation() Co-authored-by: gerrycampion <[email protected]> * revert some changes * fixes * small fixes * test fixes * remove jsonata_processor * revert some reordering * revert uneccessary move --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]>
|
Could you please resolve the conflict here? |
…uirements.txt' into 1333-sync-pyproject.toml-and-requirements.txt
No description provided.